Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

IOS/Net: implement ioctlv that retrieves the network routing table that libogc now uses on network init #13066

Merged
merged 1 commit into from
Oct 20, 2024
Merged

IOS/Net: implement ioctlv that retrieves the network routing table that libogc now uses on network init #13066

merged 1 commit into from
Oct 20, 2024

Conversation

DacoTaco
Copy link
Contributor

@DacoTaco DacoTaco commented Sep 21, 2024 •
edited
Loading

this can be tested with the following dol's
old libogc that only retrieves the IP : sockettest_org.zip
new libogc dol that retrieves the ip, subnet mask and gateway : sockettest.zip
related libogc issue : devkitPro/libogc#179

i was only able to test & implement this decently on windows, linux & android, and macos will need to have somebody that knows it look at it if it is to be implemented decently (it currently creates a fake entry for 0.0.0.0 to be sent to the retrieved gateway/broadcast ip so libogc doesn't error :) )
for macos i found this, but thats it : https://stackoverflow.com/questions/5390164/getting-routing-table-on-macosx-programmatically

@DacoTaco DacoTaco mentioned this pull request Sep 21, 2024
Merged
sepalani
sepalani suggested changes Sep 29, 2024
View reviewed changes
Copy link
Contributor

@sepalani sepalani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this PR on some online games (e.g. Monster Hunter Tri, Mario Strikers Charged Football) and didn't find regressions. Otherwise, the PR looks mostly good to me beside some of these coding style related remarks.

Regarding Android, I might be able to implement the routing table using the RouteInfo class that we're already using to get the network properties.

Source/Core/Core/IOS/Network/IP/Top.cpp Outdated Show resolved Hide resolved
Source/Core/Core/IOS/Network/IP/Top.cpp Outdated Show resolved Hide resolved
Source/Core/Core/IOS/Network/IP/Top.cpp Outdated Show resolved Hide resolved
Source/Core/Core/IOS/Network/IP/Top.cpp Outdated Show resolved Hide resolved
Source/Core/Core/IOS/Network/IP/Top.cpp Outdated Show resolved Hide resolved
Source/Core/Core/IOS/Network/IP/Top.cpp Outdated Show resolved Hide resolved
Source/Core/Core/IOS/Network/IP/Top.cpp Outdated Show resolved Hide resolved
Source/Core/Core/IOS/Network/IP/Top.cpp Outdated Show resolved Hide resolved
Source/Core/Core/IOS/Network/IP/Top.cpp Outdated Show resolved Hide resolved
Source/Core/Core/IOS/Network/IP/Top.cpp Outdated Show resolved Hide resolved
@DacoTaco DacoTaco requested a review from sepalani October 1, 2024 15:57
@JMC47
Copy link
Contributor

JMC47 commented Oct 1, 2024

@dolphin-emu-bot rebuild

sepalani
sepalani suggested changes Oct 3, 2024
View reviewed changes
Copy link
Contributor

@sepalani sepalani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the idea of using in_addr members as it makes unambiguous that the values are in network byte order. However, I believe the swaps were done in the wrong places.

Source/Core/Core/IOS/Network/IP/Top.cpp Outdated Show resolved Hide resolved
Source/Core/Core/IOS/Network/IP/Top.cpp Outdated Show resolved Hide resolved
Source/Core/Core/IOS/Network/IP/Top.cpp Outdated Show resolved Hide resolved
Source/Core/Core/IOS/Network/IP/Top.cpp Outdated Show resolved Hide resolved
Source/Core/Core/IOS/Network/IP/Top.cpp Outdated Show resolved Hide resolved
Source/Core/Core/IOS/Network/IP/Top.cpp Outdated Show resolved Hide resolved
Source/Core/Core/IOS/Network/IP/Top.cpp Outdated Show resolved Hide resolved
Source/Core/Core/IOS/Network/IP/Top.cpp Outdated Show resolved Hide resolved
Source/Core/Core/IOS/Network/IP/Top.cpp Outdated Show resolved Hide resolved
Source/Core/Core/IOS/Network/IP/Top.cpp Outdated Show resolved Hide resolved
@DacoTaco DacoTaco force-pushed the feature/networking branch 2 times, most recently from 357a785 to c3d6d2b Compare October 4, 2024 09:46
@DacoTaco DacoTaco mentioned this pull request Oct 4, 2024
Closed
@DacoTaco DacoTaco requested a review from sepalani October 6, 2024 09:54
sepalani
sepalani approved these changes Oct 8, 2024
View reviewed changes
Copy link
Contributor

@sepalani sepalani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code LGTM and tested on Windows, Linux (debian based) and Android. The Android version seems to be using the Linux part of the code and run properly on my phone (ASUS ROG Phone II). The network interface init properly for all of them.

Regarding the 2 test cases, on Linux/Android bind will fail due to privilege issues which is normal if not run as admin or with the proper capabilities set.

I guess this PR should be tested on macOS and maybe another Android device to make sure it's working properly.

@DacoTaco
Copy link
Contributor Author

DacoTaco commented Oct 8, 2024

@sepalani : confirmed working on macos, with it reporting the broadcast instead of gateway like discussed
image

sepalani
sepalani suggested changes Oct 11, 2024
View reviewed changes
Copy link
Contributor

@sepalani sepalani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding error logs might help to diagnose netlink errors, especially on Android.

Source/Core/Core/IOS/Network/IP/Top.cpp Outdated Show resolved Hide resolved
Source/Core/Core/IOS/Network/IP/Top.cpp Outdated Show resolved Hide resolved
Source/Core/Core/IOS/Network/IP/Top.cpp Outdated Show resolved Hide resolved
Source/Core/Core/IOS/Network/IP/Top.cpp Show resolved Hide resolved
@DacoTaco DacoTaco force-pushed the feature/networking branch 2 times, most recently from a4cec7b to 4c31989 Compare October 12, 2024 12:11
sepalani
sepalani suggested changes Oct 13, 2024
View reviewed changes
Copy link
Contributor

@sepalani sepalani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Other than that, tested on Windows, Linux and Android and working as expected.

Source/Core/Core/IOS/Network/IP/Top.cpp Show resolved Hide resolved
Source/Core/Core/IOS/Network/IP/Top.cpp Outdated Show resolved Hide resolved
@DacoTaco DacoTaco force-pushed the feature/networking branch 4 times, most recently from 142220e to 9f9b727 Compare October 14, 2024 20:38
sepalani
sepalani suggested changes Oct 15, 2024
View reviewed changes
Source/Core/Core/IOS/Network/IP/Top.cpp Outdated Show resolved Hide resolved
sepalani
sepalani approved these changes Oct 16, 2024
View reviewed changes
Copy link
Contributor

@sepalani sepalani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks a lot, LGTM.

Tested on Windows, Linux and Android.

@DacoTaco
Copy link
Contributor Author

DacoTaco commented Oct 18, 2024 •
edited
Loading

@JMC47 : should be ready for merging. its tested on all platforms with the test cases i provided using latest libogc builds/examples ^^;

it even works in the network tool that is on the deadly creatures disc!

@JosJuice JosJuice merged commit 24e9fc1 into dolphin-emu:master Oct 20, 2024
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JosJuice JosJuice JosJuice left review comments

@sepalani sepalani sepalani approved these changes

Assignees
No one assigned
Labels
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@DacoTaco @JMC47 @JosJuice @sepalani